Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create directory for action #752

Merged
merged 1 commit into from
Apr 1, 2024

Conversation

zbirenbaum
Copy link
Contributor

@zbirenbaum zbirenbaum commented Mar 11, 2024

Description

Modifies running actions manager and workers to create an action directory with a dedicated work directory inside instead of just creating a work directory for the task. Other directories can be mounted to this space and will be automatically cleaned on completion.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Added a test to ensure that any temporary resources in the directory are cleaned on action completion

Checklist

  • Tests added/amended
  • bazel test //... passes locally
  • PR is contained in a single commit, using git amend see some docs

This change is Reviewable

Copy link
Contributor Author

@zbirenbaum zbirenbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, publish-image, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04


nativelink-config/src/cas_server.rs line 429 at r1 (raw file):

    /// completed. This directory will be purged after the action has
    /// completed.
    action_directory,

Since this is in nativelink-config and the idea of this change is to have a space where users can put temporary resources, I thought there should be a documentation update, but there isn't any reference to these fields in the example. Should I include the v0.1 changes to deployment examples in this PR since there is one that makes use of it?

Copy link
Member

@allada allada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained


nativelink-config/src/cas_server.rs line 429 at r1 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

Since this is in nativelink-config and the idea of this change is to have a space where users can put temporary resources, I thought there should be a documentation update, but there isn't any reference to these fields in the example. Should I include the v0.1 changes to deployment examples in this PR since there is one that makes use of it?

Yeah, I think adding a sample on how to use it would be useful, but maybe this would be better to document as more of an advanced config, so users rely on it only for these special cases, like using it with docker.

or maybe in one of the examples that uses entrypoint.sh we can show it overriding TMPDIR with a path this folder?


nativelink-store/Cargo.toml line 34 at r2 (raw file):

shellexpand = "3.1.0"
tempfile = "3.10.1"
tokio = { version = "1.36.0", features = ["rt-multi-thread"] }

I don't think this is required for this change.

@zbirenbaum zbirenbaum force-pushed the root-action-directory branch 3 times, most recently from 861c459 to b791feb Compare March 25, 2024 20:01
Copy link
Contributor Author

@zbirenbaum zbirenbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, publish-image, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04


nativelink-config/src/cas_server.rs line 429 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

Yeah, I think adding a sample on how to use it would be useful, but maybe this would be better to document as more of an advanced config, so users rely on it only for these special cases, like using it with docker.

or maybe in one of the examples that uses entrypoint.sh we can show it overriding TMPDIR with a path this folder?

I can't find any examples referencing entrypoint.sh except one in terraform's GCP directory and I'm not sure exactly what to add there for this. I rebased the commit to see if any more were added and resolve the conflicts but that still seems to be the only example

Copy link
Contributor Author

@zbirenbaum zbirenbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 1 LGTMs obtained


nativelink-config/src/cas_server.rs line 429 at r1 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

I can't find any examples referencing entrypoint.sh except one in terraform's GCP directory and I'm not sure exactly what to add there for this. I rebased the commit to see if any more were added and resolve the conflicts but that still seems to be the only example

@adam-singer and I looked for a good place to include the usage or document this feature but couldn't find any. There aren't very many examples where it would make sense to set the action directory, and modifying them would likely result in users just copying it in when they go to set up their own config. Since there is not any existing documentation for the other fields in EnvironmentSource maybe we could document this feature during a full pass at those and then have a minimal example or description showing it's usage there?

Copy link
Member

@adam-singer adam-singer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 5 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: 2 of 1 LGTMs obtained

@allada allada force-pushed the main branch 2 times, most recently from a2a06c2 to 09defc6 Compare March 26, 2024 18:19
Copy link

Copy link
Member

@adam-singer adam-singer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and 2 discussions need to be resolved

@zbirenbaum zbirenbaum force-pushed the root-action-directory branch 2 times, most recently from 9f17d81 to 91f7297 Compare March 27, 2024 18:26
Copy link
Contributor Author

@zbirenbaum zbirenbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Publish image, Vercel, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, vale, and 1 discussions need to be resolved


nativelink-store/Cargo.toml line 34 at r2 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

I don't think this is required for this change.

Done.

Copy link
Contributor Author

@zbirenbaum zbirenbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 1 LGTMs obtained, and 1 discussions need to be resolved


nativelink-store/Cargo.toml line 34 at r2 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

Done.

I think this is blocking the PR since it’s the only thing left in reviewable.

Copy link
Member

@allada allada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and 1 discussions need to be resolved


nativelink-config/src/cas_server.rs line 459 at r6 (raw file):

    /// For example:
    /// If an action makes use of some constant path to create and
    /// reference files (such as /tmp), you can set this value to /tmp.

This is missleading. I suggest something like:

For example:
If an action writes temporary data to a path but nativelink should clean up this path after
the job has executed, you may create any directory under the path provided in this variable.
A common pattern would be to use `entrypoint` to set a shell script that reads this variable,
`mkdir $ENV_VAR_NAME/tmp` and `export TMPDIR=$ENV_VAR_NAME/tmp`. Another example
might be to bind-mount the `/tmp` path in a container to this path in `entrypoint`.

Modifies running actions manager and workers to create an action
directory with a dedicated work directory inside instead of just
creating a work directory for the task. Other directories can be
mounted to this space and will be automatically cleaned on completion.
Copy link
Contributor Author

@zbirenbaum zbirenbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Bazel Dev / ubuntu-22.04, Publish image, Remote / large-ubuntu-22.04, Vercel, asan / ubuntu-22.04, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, vale, and 1 discussions need to be resolved


nativelink-config/src/cas_server.rs line 459 at r6 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

This is missleading. I suggest something like:

For example:
If an action writes temporary data to a path but nativelink should clean up this path after
the job has executed, you may create any directory under the path provided in this variable.
A common pattern would be to use `entrypoint` to set a shell script that reads this variable,
`mkdir $ENV_VAR_NAME/tmp` and `export TMPDIR=$ENV_VAR_NAME/tmp`. Another example
might be to bind-mount the `/tmp` path in a container to this path in `entrypoint`.

Thanks for including this! I edited the comment.

Copy link
Member

@allada allada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! 2 of 1 LGTMs obtained

Copy link
Member

@adam-singer adam-singer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: :shipit: complete! 2 of 1 LGTMs obtained

Copy link
Contributor Author

@zbirenbaum zbirenbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 5 files at r3, 3 of 3 files at r5, 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! 2 of 1 LGTMs obtained

Copy link
Member

@allada allada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 3 files at r5.
Reviewable status: :shipit: complete! 2 of 1 LGTMs obtained

@zbirenbaum zbirenbaum merged commit 414fff3 into TraceMachina:main Apr 1, 2024
26 checks passed
@zbirenbaum zbirenbaum deleted the root-action-directory branch April 1, 2024 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants